feat: Add signal commands #20876

Legioth · 2025-01-17T16:31:32Z

This is not yet a complete signal implementation but only the low-level core data manipulation logic.

github-actions · 2025-01-17T16:47:44Z

Test Results

1 166 files + 1 1 166 suites +1 1h 39m 41s ⏱️ + 4m 38s
7 747 tests +122 7 690 ✅ +121 55 💤 - 1 0 ❌ ±0 2 🔥 +2
8 004 runs + 16 7 940 ✅ + 17 62 💤 - 3 0 ❌ ±0 2 🔥 +2

For more details on these errors, see this check.

Results for commit da62c09. ± Comparison against base commit 32c4914.

This pull request removes 2 and adds 124 tests. Note that renamed tests count towards both.

com.vaadin.flow.mixedtest.ui.IdTestIT(production) ‑ Unknown test
com.vaadin.flow.mixedtest.ui.PnpmUsedIT ‑ pnpmIsUsed

com.vaadin.signals.IdTest ‑ basicJsonSerialization
com.vaadin.signals.IdTest ‑ comparable
com.vaadin.signals.IdTest ‑ randomId_isRandom
com.vaadin.signals.IdTest ‑ zeroId_compactJson
com.vaadin.signals.impl.MutableTreeRevisionTest ‑ adoptAsCommand_aliasParent_dataNodeUpdated
com.vaadin.signals.impl.MutableTreeRevisionTest ‑ adoptAsCommand_childAdoptsItsParent_reject
com.vaadin.signals.impl.MutableTreeRevisionTest ‑ adoptAsCommand_childAlreadyInParent_keyChanged
com.vaadin.signals.impl.MutableTreeRevisionTest ‑ adoptAsCommand_childInAnotherParent_adopted
com.vaadin.signals.impl.MutableTreeRevisionTest ‑ adoptAsCommand_childMissing_reject
com.vaadin.signals.impl.MutableTreeRevisionTest ‑ adoptAsCommand_existingKey_reject
…

♻️ This comment has been updated with latest results.

taefi

Tried to do a fast review, except for the following bigger files, that will be reviewed in a second round:

MutableTreeRevision
TreeRevision
MutableTreeRevisionTest
TreeRevisionTest

signals/src/main/java/com/vaadin/signals/Node.java

taefi · 2025-01-20T09:39:44Z

signals/src/main/java/com/vaadin/signals/SignalCommand.java

+     * A signal command that doesn't apply any change but only performs a test
+     * that will be part of determining whether a transaction passes.
+     */
+    sealed interface TestCommand extends SignalCommand {


Maybe I'm a bit biased, but the name TestCommand (and further down ValueTest, PositionTest, and etc.) itches my mind. At first glance, it looks like a class that belongs to src/main/test, but in reality it is used to verify a transaction. Could this be named as VerifyCommand or something like that?

What about ConditionCommand for the base type and then naming the specific ones e.g. ValueCondition?

signals/src/main/java/com/vaadin/signals/Node.java

taefi · 2025-01-20T09:45:25Z

signals/src/main/java/com/vaadin/signals/Node.java

+         * @param listChildren
+         *            a list of child ids, or the an list if the node has no
+         *            list children


Same here. Though, I prefer to get rid of this almost duplicate block.

signals/src/main/java/com/vaadin/signals/ListSignal.java

taefi · 2025-01-20T11:55:15Z

signals/src/main/java/com/vaadin/signals/ListSignal.java

+        /**
+         * Gets the insertion position that corresponds to the beginning of the
+         * list.
+         *
+         * @return a list position for the beginning of the list, not
+         *         <code>null</code>
+         */
+        public static ListPosition first() {
+            // After edge
+            return new ListPosition(Id.ZERO, null);
+        }
+
+        /**
+         * Gets the insertion position that corresponds to the end of the list.
+         *
+         * @return a list position for the end of the list, not
+         *         <code>null</code>
+         */
+        public static ListPosition last() {
+            // Before edge
+            return new ListPosition(null, Id.ZERO);
+        }


Not sure if I easily understand this part:
The description of the after param says: id of the node to insert immediately after...
and description of the before param says: id of the node to insert immediately before...

What is the role of the ZERO? Is the ZERO an always existing root pointer that will store the head position (or the head == tail == ZERO in case of an empty list)?

If so, calling first() returns the before first (after the ZERO) position which makes sense. But, the last() returns before the ZERO, which I don't simply get :) Was it supposed to be Id.MAX?

Probably, something in the implementation is being optimized by this(?), but this representation seems a bit confusing.

This is a bit of (premature ?) optimization: ZERO is widely used also in other contexts and has therefore a custom JSON representation to be as compact as practically possible ("").

ZERO represents both edges of the list, i.e. both head and tail. This should be safe since they can never be mixed up with each other. Furthermore, ZERO also represents the root node but that node can never have siblings.

I agree that it can be confusing. I'm just not sure if that should be addressed by additional documentation or by removing the optimization? Or maybe just introduce an EDGE constant that refers to the same Id instance? It would probably be dangerous to introduce HEAD and TAIL constants with identical values since someone might assume them to be different?

Well, at this point, not seeing the rest of the implementation / usages makes it hard to make a practical decision about which approach to pick. Maybe I was mentioned and I forgot: is the List implementation going to be a circular linked list? In the case, it makes sense to have only one EDGE and the before / after seems to be enough.

What would first() and last() mean in a circular linked list?

I think, when I was writing the previous comment, I got confused for a sec with having one EDGE constant for both head and tail (for the before first and after last positions), vs. head and tail pointing to the same location all the time (which wasn't the case). I would say, having a separate constant such as EDGE is enough for not getting confused with the ZERO thingy. Having separate HEAD and TAIL constants with different values might not add much value.

signals/src/main/java/com/vaadin/signals/impl/OperationResult.java

taefi · 2025-01-20T12:21:25Z

signals/src/main/java/com/vaadin/signals/impl/OperationResult.java

+     * @return an accepted result if the condition is <code>true</code>, a
+     *         rejected result if the condition is <code>false</code>
+     */
+    public static OperationResult test(boolean condition,


Minor: suggesting to rename to something like verify:

Suggested change

public static OperationResult test(boolean condition,

public static OperationResult verify(boolean condition,

Or from, of, fromCondition, ofCondition.

taefi · 2025-01-20T13:10:54Z

signals/src/test/java/com/vaadin/signals/impl/OperationResultTest.java

+
+import com.vaadin.signals.Id;
+
+public class OperationResultTest {


There could be more tests here to have better coverage of all the implementations, especially verifying the Accept#onlyUpdate, and the test behaviour.

taefi

In this round I started by looking at Node (again), TreeRevision, and TreeRevisionTest classes, and I found myself a bit confused over the idea of mapChildren and listChildren of the Node, since in the TreeRevisionTest it distinctly uses different Ids for these two children collections, so decided to pause here for some clarifications before looking into the MutableTreeRevision.

Also, would it make sense to use either JSpecify or Jetbrains annotations for marking the NotNull params everywhere?

taefi · 2025-01-27T11:21:30Z

signals/src/main/java/com/vaadin/signals/SignalCommand.java

+     * The created node will be automatically removed if the owner is
+     * disconnected.
+     */
+    sealed interface ScopeOwnerCommand extends SignalCommand {


Suggested change

sealed interface ScopeOwnerCommand extends SignalCommand {

sealed interface ScopeOwnedCommand extends SignalCommand {

It has a field for the owner but it's owned only if that field has a non-null value.

"Owner" also doesn't completely feel right. To me it means like a command that owns the scope. I must admit it is a hard to name concept.

Based on the JavaDoc descriptions, the key ideas behind the interface are:

It creates a signal node.

The node might have an owner.

The node is automatically removed if the owner disconnects.

Fed this to GPT4o, and it suggests the following:

If the key focus is scoping the node to an owner, ScopedSignalCommand is a clear and concise option. If the focus is on automatic removal, OwnerBoundCommand or EphemeralSignalCommand could work well.

Would you like a name that emphasizes ownership, lifecycle behavior, or signal creation?

Another thing to consider is: if the owner is null, why the caller code still wants to issues this type of command to create the signal? In other words, why the owner is nullable?

In other words, why the owner is nullable?

Would be quite redundant to have separate "insert without owner" and "insert with owner" commands and similarly also for "put if absent without owner" and "put if absent with owner".

I have in general tried to keep the number of commands down and instead opted for some branching in the logic that handles those commands. That's why there's also a KeyTest command that can test for absence, presence or a specific value based on a parameter value. Those different variants will be expressed as separate methods in the high-level application-facing API even though they share the same underlying low-level command type.

ScopedCommand might be an option even though it has the same problem as ScopeOwnedCommand in not reflecting the fact that the scoping / ownership is optional.

This type is anyways mostly an internal concern that exists mainly to avoid doing two separate instanceof checks in assertValidTree. The type is exposed to application code only through the validator functionality.

signals/src/main/java/com/vaadin/signals/impl/TreeRevision.java

taefi · 2025-01-27T12:00:16Z

signals/src/main/java/com/vaadin/signals/impl/TreeRevision.java

+     *         optional of there is no node with the given id
+     */
+    public Optional<Data> data(Id id) {
+        Node node = nodes.get(id);


If the id is not supposed to be null, why not explicitly guard against it:

Suggested change

Node node = nodes.get(id);

Node node = nodes.get(Objects.requireNonNull(id, "id cannot be null."));

All paths to this method should be guarded by unit tests. Application code is not supposed to call this method.

taefi · 2025-01-27T12:43:54Z

signals/src/main/java/com/vaadin/signals/Node.java

+         *            a list of child ids, or the an list if the node has no
+         *            list children
+         * @param mapChildren
+         *            a sequenced map from key to child id, or an empty map if


What does a sequenced map mean here? Is it referring to the Java 21's SequencedMap or an ordered map? If so, shouldn't we define with a more specific interface e.g. SequencedMap or the good old LinkedHashMap?

Cannot use SequencedMap since we still support Java 17. Don't want to use LinkedHashMap in any signature since that's an implementation detail.

Then what prevents the caller code from passing an unordered map?

If caller code doesn't care about ordering, then it can do whatever it wants. But we should probably make sure there are tests to verify that order remains preserved once it has been implicitly or explicitly established.

Are those tests going to be added to this PR?

Yes. I will add those tests along with some other test omissions that have been pointed out in other comments.

taefi · 2025-01-27T14:09:38Z

signals/src/main/java/com/vaadin/signals/Node.java

+    public record Data(Id parent, Id lastUpdate, Id scopeOwner, JsonNode value,
+            List<Id> listChildren,
+            Map<String, Id> mapChildren) implements Node {


Looking at the implementation of TreeRevision # assertValidTree method, and the way it had to concat the listChildren and mapChildren, seems like a flag to me. Enabling the caller code to provide both, can easily result in creating an invalid Data node. One should be calculated based on the other, for instance, If the mapChildren changes to an orderedMap such as LinkedHashMap, then the listChildren could be just a public method that calculates based on mapChildren.values(). Or maybe I misunderstood the purpose of having both the mapChildren and listChildren side by side.

Some child nodes are accessed by key and some by order. There's no reason why a single node couldn't have children of both types even though that's not the typical case.

What I'm not understanding is: should or shouldn't the listChildren and mapChildren.values() contain the same set of Ids?

There should be no overlap. Every child should be in exactly one location - either addressable by ListPosition or by a String key but never both at the same time.

All operations that move a child remove the child from its current location before attaching it back again, even if moving within the same parent. Any accidental overlap should trigger an error in TreeRevision.assertValidTree() from the !visited.add(id) check (since the concatenation doesn't do distinct()). Should maybe add a unit test for that case as well.

There should be no overlap. Every child should be in exactly one location - either addressable by ListPosition or by a String key but never both at the same time.

To me, the it definitely worth adding this to the javadocs.

signals/src/main/java/com/vaadin/signals/impl/TreeRevision.java

Co-authored-by: Soroosh Taefi <[email protected]>

sonarqubecloud · 2025-01-28T08:49:46Z

Quality Gate passed

Issues
1433 New issues
9 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

taefi · 2025-01-28T08:59:37Z

signals/src/main/java/com/vaadin/signals/SignalCommand.java

+        /**
+         * The owner id.
+         *
+         * @return the owner id, nor <code>null</code> if the created signal has


Suggested change

* @return the owner id, nor <code>null</code> if the created signal has

* @return the owner id, or <code>null</code> if the created signal has

mshabarov

First round of review.
Will continue with the impl next and tests.

Please confirm that my following assumptions are more or less true:

Id is needed in signals to uniquely identify a signal object within some context. Whereas key is for storing signals in a Map, e.g. for signals representing each field of an entity object like Person. The signals in list and map are usually different, because in the "list" case they are of the same rank, and in the "map" case they belong to the parent object, e.g. Person.
the signal command is a command that is applied to one or more nodes and basically this is something that happens under the hood what an API user tries to get or set a value to signal or for example manipulates the collection of signals. Though the commands are not only scoped to the value operations, but also to modifications in hierarchy structure, access and transactions.
scope owners are needed for security / access layer, so that each signal allows modifications only for a given owner.
"test" commands are needed for transactions engine.

signals/src/main/java/com/vaadin/signals/Id.java

mshabarov · 2025-01-27T15:05:39Z

signals/src/main/java/com/vaadin/signals/Id.java

+    public static final Id MAX = new Id(Long.MAX_VALUE);
+
+    /*
+     * Padding refers to the trailing = characters that are only necessary when


Suggested change

* Padding refers to the trailing = characters that are only necessary when

* Padding refers to the trailing '=' characters that are only necessary when

signals/src/main/java/com/vaadin/signals/Id.java

mshabarov · 2025-01-27T16:05:29Z

signals/src/main/java/com/vaadin/signals/ListSignal.java

+     *            id of the node to insert immediately before, nor
+     *            <code>null</code> to not define a constraint
+     */
+    public record ListPosition(Id after, Id before) {


Can be marked with @Nullable from JSpecify (it should be in Flow codebase already, it was added with the CRUD repos feature)

signals/src/main/java/com/vaadin/signals/SignalCommand.java

Legioth · 2025-01-28T13:32:30Z

Id is needed in signals to uniquely identify a signal object within some context. Whereas key is for storing signals in a Map, e.g. for signals representing each field of an entity object like Person.

Correct. The id is always randomly generated (or a hardcoded constant for the root node) and carries no meaning to application logic. Application logic will very rarely even have to deal with Id values. The key is always a string provided by the application.

The signals in list and map are usually different, because in the "list" case they are of the same rang, and in the "map" case they belong to the parent object, e.g. Person.

They are all children. The only difference is whether the child is "addressed" based its position relative to other list children or addressed based on an application-provided key.

In most cases, a node has either a value, list children, or map children but not multiple types of content at the same time. This corresponds to a JSON structure where [] is a node with only list children, {} is a node with only map children and the primitive types (string, number, boolean and null) are stored as value`.

But the signal data structure is more flexible than that. Another example is XML that can be expressed so that different nodes have different meaning (though XML can also be expressed as JSON with some gotchas). A signal node that is a "list child" of another node represents an element and the signal node's value is the XML tag name. A signal node that is a "map child" of another node represents an attribute - they map key is the attribute name and the node value is the attribute value. In this way, an XML element that has both child elements and attributes will have both list children and map children.

the signal command is a command that is applied to one or more nodes and basically this is something that happens under the hood what an API user tries to get or set a value to signal or for example manipulates the collection of signals. Though the commands are not only scoped to the value operations, but also to modifications in hierarchy structure, access and transactions.

Reading values or hierarchy does not involve any commands. Commands are only for making changes (to values and/or hierarchy) and for defining conditions for transactions. And this makes me think that the XyzTest commands should be renamed to XyzCondition rather than XyZVerify that was suggested previously.

scope owners are needed for security / access layer, so that each signal allows modifications only for a given owner.

Nope. Scope owners are for automatically deleting nodes when the owner disappears (i.e. when a Hilla client is disconnected or if a Flow component is detached), even if the owner disappears in an abrupt way where it has no chance of sending cleanup commands. The main use case for this is to automatically clean up an avatar group. What would be a better name for this concept?

"test" commands are needed for transactions engine.

Confirmed. Those commands have no use on their own but only as part of a transaction that also applies some changes through "regular" commands.

Legioth · 2025-01-28T13:34:32Z

signals/src/main/java/com/vaadin/signals/SignalCommand.java

+     * A signal command that doesn't apply any change but only performs a test
+     * that will be part of determining whether a transaction passes.
+     */
+    sealed interface TestCommand extends SignalCommand {


What about ConditionCommand for the base type and then naming the specific ones e.g. ValueCondition?

signals/src/main/java/com/vaadin/signals/Id.java

signals/src/main/java/com/vaadin/signals/ListSignal.java

Legioth · 2025-01-28T14:06:53Z

signals/src/main/java/com/vaadin/signals/SignalCommand.java

+     * @param nodeId
+     *            id of the node to check, not <code>null</code>
+     * @param expectedLastUpdate
+     *            the expected id of the command hat last updated this node, not


What is "ID of the update" if not its commandId field value?

Legioth · 2025-01-28T14:07:25Z

signals/src/main/java/com/vaadin/signals/SignalCommand.java

+     * @param nodeId
+     *            id of the node to check, not <code>null</code>
+     * @param expectedLastUpdate
+     *            the expected id of the command hat last updated this node, not


Suggested change

* the expected id of the command hat last updated this node, not

* the expected id of the command that last updated this node, not

signals/src/main/java/com/vaadin/signals/SignalCommand.java

mshabarov · 2025-01-30T10:54:29Z

Nope. Scope owners are for automatically deleting nodes when the owner disappears (i.e. when a Hilla client is disconnected or if a Flow component is detached), even if the owner disappears in an abrupt way where it has no chance of sending cleanup commands. The main use case for this is to automatically clean up an avatar group. What would be a better name for this concept?

Well, with this explanation the name makes sense. I'd think of "Lifecycle" in the name, but it's nearly the same thing.

taefi

Added some questions and suggestions. The MutableTreeRevisionTest would be reviewed in the next round.

signals/src/main/java/com/vaadin/signals/impl/MutableTreeRevision.java

taefi · 2025-01-31T11:37:23Z

signals/src/main/java/com/vaadin/signals/impl/MutableTreeRevision.java

+    public void apply(List<SignalCommand> commands) {
+        for (SignalCommand command : commands) {
+            apply(command, null);
+        }
+    }


This method has no test coverage.

taefi · 2025-01-31T11:37:58Z

signals/src/test/java/com/vaadin/signals/impl/MutableTreeRevisionTest.java

+    private final Id commandId = Id.random();
+
+    @Test
+    void constuctor_modifyBase_copyNotUpdated() {


typo:

Suggested change

void constuctor_modifyBase_copyNotUpdated() {

void constructor_modifyBase_copyNotUpdated() {

taefi · 2025-01-31T11:38:10Z

signals/src/test/java/com/vaadin/signals/impl/MutableTreeRevisionTest.java

+    }
+
+    @Test
+    void constuctor_modifyCopy_baseNotUpdated() {


typo:

Suggested change

void constuctor_modifyCopy_baseNotUpdated() {

void constructor_modifyCopy_baseNotUpdated() {

taefi · 2025-01-31T11:58:07Z

signals/src/main/java/com/vaadin/signals/impl/MutableTreeRevision.java

+        OperationResult result;
+        if (data(command.nodeId()).isPresent()) {
+            TreeManipulator manipulator = new TreeManipulator(command);
+            result = manipulator.handleCommand(command);
+
+            if (manipulator.childResults != null && resultHandler != null) {
+                manipulator.childResults.forEach(resultHandler);
+            }
+        } else {
+            result = OperationResult.fail("Node not found");
+        }


Just a flavor I prefer, feel free to ignore and resolve:

Suggested change

OperationResult result;

if (data(command.nodeId()).isPresent()) {

TreeManipulator manipulator = new TreeManipulator(command);

result = manipulator.handleCommand(command);

if (manipulator.childResults != null && resultHandler != null) {

manipulator.childResults.forEach(resultHandler);

}

} else {

result = OperationResult.fail("Node not found");

}

OperationResult result = data(command.nodeId()).map(data -> {

TreeManipulator manipulator = new TreeManipulator(command);

var opResult = manipulator.handleCommand(command);

if (manipulator.childResults != null && resultHandler != null) {

manipulator.childResults.forEach(resultHandler);

}

return opResult;

}).orElseGet(() -> OperationResult.fail("Node not found"));

taefi · 2025-01-31T14:20:22Z

signals/src/test/java/com/vaadin/signals/impl/MutableTreeRevisionTest.java

+
+        // Check result object
+        Accept accept = assertAccepted(result);
+        assertEquals("Only alias is updated", 1, accept.updates().size());


Follow up for jupiter api imports:

Suggested change

assertEquals("Only alias is updated", 1, accept.updates().size());

assertEquals(1, accept.updates().size(), "Only alias is updated");

taefi · 2025-01-31T14:22:45Z

signals/src/test/java/com/vaadin/signals/impl/MutableTreeRevisionTest.java

+        assertTrue("Alias was also removed",
+                accept.updates().containsKey(alias));


Follow up for jupiter api imports:

Suggested change

assertTrue("Alias was also removed",

accept.updates().containsKey(alias));

assertTrue(accept.updates().containsKey(alias),

"Alias was also removed");

taefi · 2025-01-31T15:55:10Z

signals/src/main/java/com/vaadin/signals/impl/MutableTreeRevision.java

+        if (result instanceof Accept accept) {
+            accept.updates().forEach((nodeId, update) -> {
+                Node newNode = update.newNode();
+
+                if (newNode == null) {
+                    nodes().remove(nodeId);
+                    originalInserts().remove(nodeId);
+                } else {
+                    nodes().put(nodeId, newNode);
+                }
+            });
+
+            originalInserts().putAll(accept.originalInserts());
+        }


As I don't see what happens to a MutableTreeRevision instance after a command is applied to it, I still don't have a clear understanding of why originalInserts().remove(nodeId); should happen when a command ends up removing a node from the tree.

When the scope object that owns a node no longer exists, If the node(s) owned by that owner should be removed from the tree. Then, according to the javadoc of TreeRevision # originalInserts, some node that is no longer exist as a Data node should be added back again when the owner is back, by replaying the same command from the original insert collection.

I grasp that If there's no removal from that collection, it grows forever, and it doesn't make sense, but at the same time, without knowing how the system is going to notify the absence of an owner to a tree, I cannot digest the logic of removing from originalInserts() every time a command results in removing a data node.

When commented out the originalInserts().remove(nodeId);, only those tests that are related to the removal of scope owned nodes are failing, so it is a bit confusing.

I couldn't get why originalInserts().remove(nodeId); is needed too.
Original inserts in accept.originalInserts() are only added in createNode() by PutIfAbsentCommand or InsertCommand, but never removed from there, so even if some inserts are removed from map of MutableTreeRevision, they are still held in map of TreeManipulator and re-added.

Maybe they are for different nodes, but I couldn't deduct this from the codes.

Would leak memory if we don't remove those entires when the associated node is removed. The "happy path" is that the node is indeed supposed to be removed for good and then this is the right place to do it.

The other case is when it's only a temporary removal due to cleanup when the node owner is disconnected which means that the node owner may need it again later if it gets reconnected. To deal with that, the node owner needs to create a copy of its original inserts in the logic where it handles being disconnected and reconnected so it's outside the logic that exists in this PR.

taefi · 2025-01-31T16:19:02Z

signals/src/main/java/com/vaadin/signals/impl/MutableTreeRevision.java

+                String key = parentData.mapChildren().entrySet().stream()
+                        .filter(entry -> entry.getValue().equals(id))
+                        .map(Entry::getKey).findAny().orElse(null);


Nit: Prioritize findAny() before mapping:

Suggested change

String key = parentData.mapChildren().entrySet().stream()

.filter(entry -> entry.getValue().equals(id))

.map(Entry::getKey).findAny().orElse(null);

String key = parentData.mapChildren().entrySet().stream()

.filter(entry -> entry.getValue().equals(id))

.findAny().map(Entry::getKey).orElse(null);

taefi · 2025-01-31T16:27:45Z

signals/src/main/java/com/vaadin/signals/impl/MutableTreeRevision.java

+            }
+
+            useData(parentId, (node, id) -> {
+                detachedNodes.remove(resolvedChildId);


Should detachedNodes.remove(resolvedChildId); for some reason be happening within the lambda that has no dependency to?

There's no direct data relationship but useData checks for an error condition and we shouldn't apply any changes before that check has been performed.

Maybe worth having a comment about why it is called there.

mshabarov

Second round of review.
I'm focusing now on the MutableTreeRevision.

mshabarov · 2025-01-30T09:20:12Z

signals/src/main/java/com/vaadin/signals/impl/TreeRevision.java

+     * <ul>
+     * <li>All nodes are attached to the root
+     * <li>All parent-child relationships are consistent in both directions
+     * <li>No node is attached in multiple places
+     * <li>All aliases target an existing data node
+     * <li>All nodes with a matching scope owner has a matching original insert
+     * <li>All original insert entries correspond to a node with a matching
+     * scope owner
+     * </ul>


How nodes map and scope owner map related to each other:

Should they always have the same set of Id's, i.e. maps sizes are same always ?

Can it be so that some nodes have no scope owner, i.e. no initial insert?

Scope owner is optional so most nodes will not have a scope owner and there will thus not be any initial insert for most nodes. Furthermore, the initial insert is only needed for the tree revision instance that owns a node whereas it would be redundant for others to keep track of it.

mshabarov · 2025-01-30T11:59:40Z

signals/src/main/java/com/vaadin/signals/SignalCommand.java

+     * @param nodeId
+     *            id of the node to check, not <code>null</code>
+     * @param expectedLastUpdate
+     *            the expected id of the command hat last updated this node, not


In other words what is the difference between commandId and expectedLastUpdate, if the javadoc calls them both "id of the command" ?

I would guess that the commandId is the type of the command and expectedLastUpdate is incremental id of the command of a given type, is that correct?

mshabarov · 2025-01-30T12:32:53Z

signals/src/main/java/com/vaadin/signals/impl/OperationResult.java

+ * @see SignalCommand
+ * @see TreeRevision
+ */
+public sealed interface OperationResult {


The name could be CommandResult to have a better match with the SignalCommand.

mshabarov · 2025-01-30T13:01:58Z

signals/src/main/java/com/vaadin/signals/impl/OperationResult.java

+     *            the new node instance or null if the operation removed the
+     *            node
+     */
+    record TreeModification(Node oldNode, Node newNode) {


I would prefer NodeModification name instead, but this is minor.

mshabarov · 2025-01-30T13:32:13Z

signals/src/main/java/com/vaadin/signals/impl/OperationResult.java

+     * @return an accepted result if the condition is <code>true</code>, a
+     *         rejected result if the condition is <code>false</code>
+     */
+    public static OperationResult test(boolean condition,


Or from, of, fromCondition, ofCondition.

signals/src/main/java/com/vaadin/signals/impl/Snapshot.java

mshabarov · 2025-02-03T07:49:28Z

signals/src/main/java/com/vaadin/signals/impl/MutableTreeRevision.java

+        if (result instanceof Accept accept) {
+            accept.updates().forEach((nodeId, update) -> {
+                Node newNode = update.newNode();
+
+                if (newNode == null) {
+                    nodes().remove(nodeId);
+                    originalInserts().remove(nodeId);
+                } else {
+                    nodes().put(nodeId, newNode);
+                }
+            });
+
+            originalInserts().putAll(accept.originalInserts());
+        }


I couldn't get why originalInserts().remove(nodeId); is needed too.
Original inserts in accept.originalInserts() are only added in createNode() by PutIfAbsentCommand or InsertCommand, but never removed from there, so even if some inserts are removed from map of MutableTreeRevision, they are still held in map of TreeManipulator and re-added.

Maybe they are for different nodes, but I couldn't deduct this from the codes.

Add signal commands

95949da

This is not yet a complete signal implementation but only the low-level core data manipulation logic.

Legioth requested review from mshabarov and taefi January 17, 2025 16:32

vaadin-bot added the +0.0.1 label Jan 17, 2025

Legioth added 2 commits January 17, 2025 18:56

Add some missing Javadocs

eea8740

Add license headers

371348d

mshabarov changed the title ~~Add signal commands~~ feat: Add signal commands Jan 20, 2025

taefi requested changes Jan 20, 2025

View reviewed changes

taefi requested changes Jan 27, 2025

View reviewed changes

Apply suggestions from code review

da62c09

Co-authored-by: Soroosh Taefi <[email protected]>

vaadin-bot added +1.0.0 and removed +0.0.1 labels Jan 28, 2025

taefi reviewed Jan 28, 2025

View reviewed changes

mshabarov reviewed Jan 28, 2025

View reviewed changes

Legioth commented Jan 28, 2025

View reviewed changes

taefi requested changes Jan 31, 2025

View reviewed changes

mshabarov reviewed Feb 3, 2025

View reviewed changes

	public static OperationResult test(boolean condition,
	public static OperationResult verify(boolean condition,


		import com.vaadin.signals.Id;

		public class OperationResultTest {

	sealed interface ScopeOwnerCommand extends SignalCommand {
	sealed interface ScopeOwnedCommand extends SignalCommand {

	Node node = nodes.get(id);
	Node node = nodes.get(Objects.requireNonNull(id, "id cannot be null."));

	* @return the owner id, nor <code>null</code> if the created signal has
	* @return the owner id, or <code>null</code> if the created signal has

	* Padding refers to the trailing = characters that are only necessary when
	* Padding refers to the trailing '=' characters that are only necessary when

	* the expected id of the command hat last updated this node, not
	* the expected id of the command that last updated this node, not

	void constuctor_modifyBase_copyNotUpdated() {
	void constructor_modifyBase_copyNotUpdated() {

	void constuctor_modifyCopy_baseNotUpdated() {
	void constructor_modifyCopy_baseNotUpdated() {

	assertEquals("Only alias is updated", 1, accept.updates().size());
	assertEquals(1, accept.updates().size(), "Only alias is updated");

		assertTrue("Alias was also removed",
		accept.updates().containsKey(alias));

feat: Add signal commands #20876

Are you sure you want to change the base?

feat: Add signal commands #20876

Conversation

Legioth commented Jan 17, 2025

github-actions bot commented Jan 17, 2025 • edited Loading

Test Results

taefi left a comment • edited Loading

Choose a reason for hiding this comment

taefi Jan 20, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

taefi Jan 20, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

taefi Jan 28, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

taefi left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

taefi Jan 28, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

taefi Jan 28, 2025 • edited Loading

Choose a reason for hiding this comment

sonarqubecloud bot commented Jan 28, 2025

Quality Gate passed

Choose a reason for hiding this comment

mshabarov left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Legioth commented Jan 28, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mshabarov commented Jan 30, 2025

taefi left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

taefi Jan 31, 2025 • edited Loading

Choose a reason for hiding this comment

taefi Jan 31, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mshabarov left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Jan 17, 2025 •

edited

Loading

taefi left a comment •

edited

Loading

taefi Jan 20, 2025 •

edited

Loading

taefi Jan 20, 2025 •

edited

Loading

taefi Jan 28, 2025 •

edited

Loading

taefi left a comment •

edited

Loading

taefi Jan 28, 2025 •

edited

Loading

taefi Jan 28, 2025 •

edited

Loading

mshabarov left a comment •

edited

Loading

taefi Jan 31, 2025 •

edited

Loading

taefi Jan 31, 2025 •

edited

Loading